fix(wireprotocol): only send bypassDocumentValidation if true #317
Conversation
The bypassDocumentValidation key will only be set if it explicitly receives a true, otherwise it remains undefined. Fixes NODE-1492
| const wireProtocol2_6 = require('../../../lib/wireprotocol/2_6_support.js'); | ||
| const wireProtocol3_2 = require('../../../lib/wireprotocol/3_2_support.js'); | ||
|
|
||
| const MockPool = {}; |
There was a problem hiding this comment.
can we actually make this a class MockPool { write() { ... } } so we can potentially reuse it for other purposes in the future?
|
|
||
| describe('WireProtocol', function() { | ||
| it('2.6 should only set bypassDocumentValidation to true if explicitly set by user to true', function(done) { | ||
| const options = { bypassDocumentValidation: true }; |
There was a problem hiding this comment.
here you would create a const pool = new MockPool( /* maybe future options */ ) and pass that into the insert method.
| describe('WireProtocol', function() { | ||
| it('2.6 should only set bypassDocumentValidation to true if explicitly set by user to true', function(done) { | ||
| const options = { bypassDocumentValidation: true }; | ||
| wireProtocol2_6.prototype.insert(MockPool, '', '', bson, 'ops', options, function(err, data) { |
There was a problem hiding this comment.
similarly I would recommend creating an instance of the WireProtocol: const wireProtocol = new WireProtocol2_6(); and using wireProtocol.insert
| describe('WireProtocol', function() { | ||
| it('2.6 should only set bypassDocumentValidation to true if explicitly set by user to true', function(done) { | ||
| const options = { bypassDocumentValidation: true }; | ||
| wireProtocol2_6.prototype.insert(MockPool, '', '', bson, 'ops', options, function(err, data) { |
There was a problem hiding this comment.
finally, I think for readability-sake in the future it would be best to name the variables you are passing in fake data for, e.g.
const ns = 'fake.namespace';
const ismaster = {};
...
wireProtocol.insert(pool, ns, ismaster, ...)
There was a problem hiding this comment.
You are currently reusing fake_bson, fake_topology, and pool across all the tests. This is not advisable, as spies maintain state. You should do one of the following:
- Move the creation of the mocks to within
testPoolWrite - Create a
beforeEachhook that creates a newfake_bson,fake_topology, andpoolfor each test.
| MockPool.write = function(cmd, options, callback) { | ||
| callback(null, cmd); | ||
| const fake_topology = 'fake_topology'; | ||
| const fake_bson = { |
There was a problem hiding this comment.
You could probably replace this with sinon.createStubInstance(bson).
| }; | ||
|
|
||
| const pool = new Pool(fake_topology, { bson: fake_bson }); |
There was a problem hiding this comment.
these two lines could be const pool = sinon.createStubInstance(Pool)
| function testPoolWrite(bypassDocumentValidation, wireProtocol, expected) { | ||
| const isMaster = {}; | ||
| const ns = 'fake.namespace'; | ||
| const ops = 'fake.ops'; |
There was a problem hiding this comment.
ops should be an array of operations. Ideally, you would do something like:
const ops = [ {a: 1}, {b: 2} ];|
|
||
| MockPool.write = function(cmd, options, callback) { | ||
| callback(null, cmd); | ||
| const fake_topology = 'fake_topology'; |
There was a problem hiding this comment.
The topology is normally an object. I don't think it is worth it to stub the whole topology, but it should probably be an empty object {} instead of a string.
The bypassDocumentValidation key will only be set if it explicitly
receives a true, otherwise it remains undefined.
Fixes NODE-1492